Add support for lossy HTJ2K compressor#2425
Conversation
|
@Vertexwahn, any suggestion to fix the Bazel failure here? I see two errors in the logs: and:
|
|
And @palemieux, can you amend the commits with |
|
Short time solution: Change in |
|
Once bazelbuild/bazel-central-registry#9032 is merge you can change |
palemieux
force-pushed
the
feature/lossy-htj2k
branch
2 times, most recently
from
cad022b to
b87f94b
Compare
Signed-off-by: Pierre-Anthony Lemieux <pal@sandflow.com>
Signed-off-by: Pierre-Anthony Lemieux <pal@sandflow.com>
palemieux
force-pushed
the
feature/lossy-htj2k
branch
from
b87f94b to
5011ecd
Compare
| { | ||
| float a = static_cast<float> (orig[px]); | ||
| float b = static_cast<float> (rereadPx[px]); | ||
| if (std::isfinite (a) && std::isfinite (b)) |
There was a problem hiding this comment.
please add code comments on why divide by 0.0000001 is necessary
| } | ||
|
|
||
| metrics.stats[p].mseCount = count; | ||
| metrics.stats[p].mse = |
There was a problem hiding this comment.
to make this more clear, the variable mse could be changed to logmse
|
It might be better to put the |
| "$<BUILD_INTERFACE:${_openexr_imath_cfg_compat}>") | ||
| endif() | ||
| set(OPENEXR_IMATH_SUBDIR_INCLUDE_PATCH "${_openexr_imath_inc_patch_key}" CACHE INTERNAL "") | ||
| target_include_directories(Imath INTERFACE |
There was a problem hiding this comment.
This fix mirrors what I just submitted in #2444, I think. Fine to leave it here.
| float& dwaCompressionLevel (); | ||
| IMF_EXPORT | ||
| float dwaCompressionLevel () const; | ||
| IMF_EXPORT |
There was a problem hiding this comment.
You're probably just following the pattrn of dwa and zip, but is there a reason this is not just stored as a regular float attribute in the header?
There was a problem hiding this comment.
Just following the pattern. I am not convinced that there is a reason to store the quality level in the header.
cary-ilm
left a comment
cary-ilm
left a comment
There was a problem hiding this comment.
Just to confirm, are the changes to the vendored OpenJPH simply from vendoring in 0.27.3? Are there any changes we need to make sure don't get stomped on?
Stock 27.3. No additional changes. Thanks for making sure. |
Ok. Will split it off before the lossy J2K branch is merged. |
|
|
||
| exr_set_zip_compression_level (_ctxt, 0, hdr.zipCompressionLevel ()); | ||
| exr_set_dwa_compression_level (_ctxt, 0, hdr.dwaCompressionLevel ()); | ||
| exr_set_lossy_htj2k_quality (_ctxt, 0, hdr.lossyHTJ2KQuality ()); |
There was a problem hiding this comment.
| exr_set_lossy_htj2k_quality (_ctxt, 0, hdr.lossyHTJ2KQuality ()); | |
| exr_set_htj2k_compression_quality (_ctxt, 0, hdr.HTJ2KCompressionQuality ()); |
HTJ2K Compression Quality
There was a problem hiding this comment.
I prefer the existing names that include "lossy", to distinguish it from the non-lossy htj2k
|
@peterhillman and @palemieux, since this PR is going into the lossy-htj2k feature branch, the exrmetrics changes can stay here and also go into a separate PR for main. That will highlight them more prominently in the commit history and release notes. |
|
Ok. I can create a PR against main. |
|
@palemieux, I think this is only waiting on @michaeldsmith's comment suggestions? |
|
See #2448 for a standalone PR adding MSE to exrmetrics. |
6 participants
See https://academysoftwarefdn.slack.com/archives/CMLRW4N73/p1777317753088359